fix: block critical recursive delete paths#524
Conversation
📝 WalkthroughWalkthrough
ChangesRecursive-delete policy
Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Automated code review started - full review. Results will be posted here. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
__tests__/hooks/builtin-policies.test.ts (1)
621-630: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for
$HOME/relative paths and multiple slashes.To ensure the edge-case bypasses identified in the implementation review are prevented, consider adding these permutations to the parameterized tests.
♻️ Proposed test additions
it.each([ "rm -rf $HOME", "rm -rf ${HOME}", + "rm -rf $HOME/Documents", "rm -rf ~/Documents", + "rm -rf ~//Documents", "rm -rf /home/chetan", "rm -rf /var/lib", + "rm -rf //var/lib", ])("blocks critical recursive-delete target %s", async (command) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/hooks/builtin-policies.test.ts` around lines 621 - 630, Extend the parameterized cases in the “blocks critical recursive-delete target” test to cover recursive-delete commands using the relative `$HOME/` form and paths containing multiple slashes. Keep the existing context setup and deny assertion unchanged, ensuring both added permutations are rejected by policy.fn.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/builtin-policies.ts`:
- Around line 649-653: Replace the raw-command regex in the find deletion check
with parsed-token analysis: identify an actual find executable invocation,
require a -delete argument, and validate its target paths using the same
critical-path logic as rm handling (including paths such as // and /var/lib).
Preserve the existing deny response while avoiding matches inside quoted
arguments or unrelated commands.
- Around line 634-644: Update isCriticalRecursiveDeletePath to collapse
consecutive slashes in the normalized path before applying the protected-path
checks, so inputs such as //usr and /var//lib match the existing rules. Extend
the home-relative checks to recognize paths beginning with $HOME/ and ${HOME}/
in addition to ~/ while preserving the existing exact home-token handling.
---
Nitpick comments:
In `@__tests__/hooks/builtin-policies.test.ts`:
- Around line 621-630: Extend the parameterized cases in the “blocks critical
recursive-delete target” test to cover recursive-delete commands using the
relative `$HOME/` form and paths containing multiple slashes. Keep the existing
context setup and deny assertion unchanged, ensuring both added permutations are
rejected by policy.fn.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3323c33-ff33-4b23-a793-c18560c08d85
📒 Files selected for processing (2)
__tests__/hooks/builtin-policies.test.tssrc/hooks/builtin-policies.ts
| function isCriticalRecursiveDeletePath(token: string): boolean { | ||
| const normalized = token.replace(/\/\*$/, "").replace(/\/+$/, "") || (token.startsWith("/") ? "/" : ""); | ||
| return normalized === "/" | ||
| || normalized === "~" | ||
| || normalized === "$HOME" | ||
| || normalized === "${HOME}" | ||
| || normalized.startsWith("~/") | ||
| || /^\/[A-Za-z_][\w.-]*$/.test(normalized) | ||
| || /^\/(?:home|Users)\/[^/]+$/.test(normalized) | ||
| || /^\/(?:var\/(?:lib|log)|usr\/(?:lib|local))$/.test(normalized); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fix path normalization to prevent bypasses and include $HOME relative paths.
- Multiple slashes bypass: Paths like
//usr,//var/lib, or/var//libwill bypass the exact regex matches because multiple consecutive slashes are not collapsed (e.g.,//usris perfectly valid in Bash but fails/^\/[A-Za-z_]/). - Missing
$HOME/coverage: The logic correctly covers~/withnormalized.startsWith("~/"), but misses$HOME/and${HOME}/. This leavesrm -rf $HOME/Documentsexposed, contradicting the PR objective to protect home-relative paths.
Normalize paths by collapsing consecutive slashes, and add checks for $HOME/ and ${HOME}/.
🛡️ Proposed fix
function isCriticalRecursiveDeletePath(token: string): boolean {
- const normalized = token.replace(/\/\*$/, "").replace(/\/+$/, "") || (token.startsWith("/") ? "/" : "");
+ const normalized = token.replace(/\/{2,}/g, "/").replace(/\/\*$/, "").replace(/\/+$/, "") || (token.startsWith("/") ? "/" : "");
return normalized === "/"
|| normalized === "~"
|| normalized === "$HOME"
|| normalized === "${HOME}"
|| normalized.startsWith("~/")
+ || normalized.startsWith("$HOME/")
+ || normalized.startsWith("${HOME}/")
|| /^\/[A-Za-z_][\w.-]*$/.test(normalized)
|| /^\/(?:home|Users)\/[^/]+$/.test(normalized)
|| /^\/(?:var\/(?:lib|log)|usr\/(?:lib|local))$/.test(normalized);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function isCriticalRecursiveDeletePath(token: string): boolean { | |
| const normalized = token.replace(/\/\*$/, "").replace(/\/+$/, "") || (token.startsWith("/") ? "/" : ""); | |
| return normalized === "/" | |
| || normalized === "~" | |
| || normalized === "$HOME" | |
| || normalized === "${HOME}" | |
| || normalized.startsWith("~/") | |
| || /^\/[A-Za-z_][\w.-]*$/.test(normalized) | |
| || /^\/(?:home|Users)\/[^/]+$/.test(normalized) | |
| || /^\/(?:var\/(?:lib|log)|usr\/(?:lib|local))$/.test(normalized); | |
| } | |
| function isCriticalRecursiveDeletePath(token: string): boolean { | |
| const normalized = token.replace(/\/{2,}/g, "/").replace(/\/\*$/, "").replace(/\/+$/, "") || (token.startsWith("/") ? "/" : ""); | |
| return normalized === "/" | |
| || normalized === "~" | |
| || normalized === "$HOME" | |
| || normalized === "${HOME}" | |
| || normalized.startsWith("~/") | |
| || normalized.startsWith("$HOME/") | |
| || normalized.startsWith("${HOME}/") | |
| || /^\/[A-Za-z_][\w.-]*$/.test(normalized) | |
| || /^\/(?:home|Users)\/[^/]+$/.test(normalized) | |
| || /^\/(?:var\/(?:lib|log)|usr\/(?:lib|local))$/.test(normalized); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/builtin-policies.ts` around lines 634 - 644, Update
isCriticalRecursiveDeletePath to collapse consecutive slashes in the normalized
path before applying the protected-path checks, so inputs such as //usr and
/var//lib match the existing rules. Extend the home-relative checks to recognize
paths beginning with $HOME/ and ${HOME}/ in addition to ~/ while preserving the
existing exact home-token handling.
| const hasDestructivePath = parseArgvTokens(cmd).some(isCriticalRecursiveDeletePath); | ||
|
|
||
| if (/\bfind\s+(?:\/|~|\$HOME|\$\{HOME\})(?=\s|$)[\s\S]*?\s-delete\b/.test(cmd)) { | ||
| return deny("Catastrophic deletion blocked"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Refine find -delete detection to avoid false positives and bypasses.
The current regex for find operates on the raw cmd string:
- False positives: Commands like
echo "find / -delete"orgit commit -m "find / -delete"will incorrectly trigger a block because token boundaries and quotes are ignored. - Bypasses:
find // -deletebypasses the exact string match. - Inconsistent coverage: Hardcoding
(?:\/|~|\$HOME|\$\{HOME\})misses other critical paths (like/var/lib) that are correctly protected againstrm -rf.
You can fix all of these by leveraging the parsed tokens to verify that find and -delete are actual executable tokens, holding find target paths to the exact same standard as rm.
🛡️ Proposed fix
- const hasDestructivePath = parseArgvTokens(cmd).some(isCriticalRecursiveDeletePath);
-
- if (/\bfind\s+(?:\/|~|\$HOME|\$\{HOME\})(?=\s|$)[\s\S]*?\s-delete\b/.test(cmd)) {
- return deny("Catastrophic deletion blocked");
- }
+ const tokens = parseArgvTokens(cmd);
+ const hasDestructivePath = tokens.some(isCriticalRecursiveDeletePath);
+
+ const findIdx = tokens.indexOf("find");
+ const deleteIdx = tokens.indexOf("-delete");
+ if (findIdx !== -1 && deleteIdx > findIdx) {
+ const findArgs = tokens.slice(findIdx + 1, deleteIdx);
+ if (findArgs.some(isCriticalRecursiveDeletePath)) {
+ return deny("Catastrophic deletion blocked");
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const hasDestructivePath = parseArgvTokens(cmd).some(isCriticalRecursiveDeletePath); | |
| if (/\bfind\s+(?:\/|~|\$HOME|\$\{HOME\})(?=\s|$)[\s\S]*?\s-delete\b/.test(cmd)) { | |
| return deny("Catastrophic deletion blocked"); | |
| } | |
| const tokens = parseArgvTokens(cmd); | |
| const hasDestructivePath = tokens.some(isCriticalRecursiveDeletePath); | |
| const findIdx = tokens.indexOf("find"); | |
| const deleteIdx = tokens.indexOf("-delete"); | |
| if (findIdx !== -1 && deleteIdx > findIdx) { | |
| const findArgs = tokens.slice(findIdx + 1, deleteIdx); | |
| if (findArgs.some(isCriticalRecursiveDeletePath)) { | |
| return deny("Catastrophic deletion blocked"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/builtin-policies.ts` around lines 649 - 653, Replace the
raw-command regex in the find deletion check with parsed-token analysis:
identify an actual find executable invocation, require a -delete argument, and
validate its target paths using the same critical-path logic as rm handling
(including paths such as // and /var/lib). Preserve the existing deny response
while avoiding matches inside quoted arguments or unrelated commands.
|
Review failed: Could not clone repository. |
|
Your PR is awaiting review by a moderator. Till then you can join the Discord for conversation: https://discord.gg/qaFM2uYFb |
|
Your PR is awaiting review by a moderator. Till then you can join the Discord for conversation: https://discord.befailproof.ai |
|
Your PR is awaiting review by a reviewer. Till then you can join the Discord for conversation: https://discord.befailproof.ai |
|
Automated code review started - full review. Results will be posted here. |
|
Phase 2 in progress - deep code analysis. Detected a focused security-focused PR: extends block-rm-rf with critical path protection. Continuing to Phase 3 (build + test) next. |
2 participants
What
Extends
block-rm-rfcoverage to deny recursive deletion of critical paths:$HOME,${HOME}, and home-relative paths such as~/Documents/home/<user>and/Users/<user>/var/lib,/var/log,/usr/lib, and/usr/localfind / -deleteand equivalent home-root targetsAn explicit
allowPathsentry still permits the configured home-relative path.Why
The policy previously recognized only the root,
~, and single-segment absolute paths. Common destructive forms could bypass it while ordinary project cleanup should remain allowed.Addresses #520.
Validation
npx vitest run __tests__/hooks/builtin-policies.test.ts -t "block-rm-rf"— 37 passednpx eslint src/hooks/builtin-policies.ts __tests__/hooks/builtin-policies.test.tsnpx tsc --noEmitThe full policy test file still has an unrelated pre-existing failure in
block-read-outside-cwdallow-path handling. Docker smoke testing was unavailable because Docker Desktop's local API did not become ready.Summary by CodeRabbit
find ... -deletecommands targeting critical directories.~/scratch, remain permitted.